Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for StreamingCredentials #3068

Closed
wants to merge 21 commits into from
Closed

Conversation

ggivo
Copy link
Contributor

@ggivo ggivo commented Dec 2, 2024

Description
This PR is preparation step for adding support for Microsoft Entra ID (formerly Azure AD) authentication support

It introduces support for streaming credentials provider allowing connection re-authentication when new credentials are available.
Posible use cases is token based authentication (e.g. expiring tokens) or credential rotation.

PR introduces :

  1. StreamingCredentialsProvider extending existing CredentialsProvider with option for providing a stream of credentials
  2. If enabled connection will subscribe for credential updates and perform re-authentication

An example of credential provider supporting streaming : MyStreamingRedisCredentialsProvider
A basic usage would look like;

        MyStreamingRedisCredentialsProvider credentialsProvider = new MyStreamingRedisCredentialsProvider();
        
        RedisURI uri = RedisURI.builder()
            .withHost("REDIS_HOST")
            .withPort("REDIS_PORT")
            // use credential provider supporting credential stream
            .withAuthentication(credentialsProvider).build();  

        RedisClient client = RedisClient.create(uri);
        client.setOptions(ClientOptions.builder()
                 //enable re-authentication on new credentials
                .reauthenticateBehavior(ClientOptions.ReauthenticateBehavior.ON_NEW_CREDENTIALS)  
                .build());

        connection = client.connect()  
        ....
  • You have read the contribution guidelines.
  • You have created a feature request first to discuss your contribution intent. Please reference the feature request ticket number in the pull request.
  • You applied code formatting rules using the mvn formatter:format target. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.

     This enables use cases like credential rotation and token based auth without client disconnect. Especially with Pub/Sub clients will reduce the chnance of missing events.
@ggivo ggivo self-assigned this Dec 4, 2024
@ggivo ggivo requested a review from tishun December 4, 2024 09:52
@ggivo ggivo marked this pull request as ready for review December 4, 2024 09:53
ggivo added a commit to ggivo/lettuce that referenced this pull request Dec 4, 2024
ggivo added 2 commits December 8, 2024 14:09
  - Moved Authentication handler to DefaultEndpoint
  - updated since 6.6.0
ggivo added a commit to ggivo/lettuce that referenced this pull request Dec 8, 2024
# Conflicts:
#	src/test/java/io/lettuce/core/AuthenticationIntegrationTests.java
ggivo added a commit to ggivo/lettuce that referenced this pull request Dec 8, 2024
# Conflicts:
#	src/test/java/io/lettuce/core/AuthenticationIntegrationTests.java
ggivo added a commit to ggivo/lettuce that referenced this pull request Dec 8, 2024
# Conflicts:
#	src/test/java/io/lettuce/core/AuthenticationIntegrationTests.java
@ggivo ggivo marked this pull request as draft December 11, 2024 12:02
@ggivo ggivo force-pushed the streaming-auth branch 2 times, most recently from d483898 to 7185e22 Compare December 11, 2024 17:32
Defer the re-auth operation in case there is on-going multi
Tx in lettuce need to be externally synchronised when used in multithreaded env. Since re-auth happens from different thread we need to make sure it does not happen while there is ongoing transaction.
@ggivo ggivo marked this pull request as ready for review December 13, 2024 11:51
@ggivo ggivo requested a review from atakavci December 13, 2024 12:17
Copy link
Collaborator

@atakavci atakavci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks clean from my POV, i got a few questions though.

}

if (credentialsProvider instanceof StreamingCredentialsProvider) {
if (!isSupportedConnection()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we good with the behavior when its configured with StreamingCredentials but doesnt work that way. It looks like it happens silently and nobody knows about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed that one. WIll add a warning to the log.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, this is expected. We can have a streaming-enabled credentials provider and disabled the reauthentication

multi = (multi == null ? new MultiOutput<>(codec) : multi);

if (command instanceof CompleteableCommand) {
((CompleteableCommand<?>) command).onComplete((ignored, e) -> {
if (e != null) {
multi = null;
inTransaction.set(false);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what are the cases that we know of it is not a CompleteableCommand ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code you can provide directly Command to dispath.
What is the actual use? I am not sure but if it is not CompletableCommand it will make most of preProcessCommand() method irrelevant.
Not sure if @tishun can add here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷

I thought it was that all sync commands are not completable commands.
But why are they exempt from these checks I am not sure.

Does MULTI even work with sync commands?

Copy link
Contributor Author

@ggivo ggivo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tishun
Thanks for the review and help with clean up! It looks neat and tidy!

I think there a few changes that lead to change in behavior
Putting some comments and will try to address them with follow up commit.

move credentials() method to RedisCredentialsProvider.

Resolve issue with unsafe cast after extending RedisCredentialsProvider with supportsStreaming() method
Copy link
Collaborator

@tishun tishun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tishun
Copy link
Collaborator

tishun commented Dec 23, 2024

Hey @ggivo - i guess we can close this one, right?
If so please close it. Thanks!

@ggivo ggivo closed this Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants